fix!: flagd, behavioral tests conformance (evaluation and config) #76
Conversation
Summary of ChangesHello @erenatas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the project by updating to the Rust 2024 edition and bringing various dependencies to their latest versions. It introduces significant enhancements to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a number of significant improvements and fixes across the flagd and env-var crates. Key changes include upgrading to the Rust 2024 edition, updating numerous dependencies, and most notably, a major refactoring of the in-process targeting engine to use datalogic-rs v4. This refactoring has greatly improved the code's safety and clarity, especially in the fractional and semver targeting operators.
The behavioral tests have also been substantially improved for better conformance and reliability. The use of test-with and temp-env to manage environment variables in tests is a great change that addresses potential flakiness from parallel test execution.
I have one suggestion in crates/flagd/src/resolver/in_process/resolver/grpc.rs to improve code consistency in cache handling, bringing it in line with the FileResolver. Overall, this is a high-quality pull request with excellent changes.
Signed-off-by: Eren Atas <[email protected]>
Signed-off-by: Eren Atas <[email protected]>
Signed-off-by: Eren Atas <[email protected]>
Signed-off-by: Eren Atas <[email protected]>
Signed-off-by: Eren Atas <[email protected]>
Signed-off-by: Eren Atas <[email protected]>
Signed-off-by: Eren Atas <[email protected]>
Signed-off-by: Eren Atas <[email protected]>
erenatas
force-pushed
the
eren/behavioral-conformance-1
branch
from
3142335 to
f5c6a3b
Compare
erenatas
changed the title
Signed-off-by: Eren Atas <[email protected]>
beeme1mr
changed the title
Signed-off-by: Eren Atas <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements by fixing a configuration precedence issue and adding comprehensive behavioral tests for both configuration and evaluation. The changes make the provider more robust and conformant with specifications.
The new provider_id option is a good addition for identifying provider instances. The error mapping from gRPC status codes to OpenFeature error codes is a great enhancement for error handling consistency. The new evaluation test suite using testcontainers and Gherkin is particularly impressive and greatly increases confidence in the provider's correctness.
I have two main points of feedback:
- A potential discrepancy in the configuration test logic which seems to contradict the intended precedence rules.
- An opportunity to reduce code duplication in the new evaluation test suite.
Overall, this is a high-quality contribution. Please see my detailed comments.
| if let Some(source) = world.option_values.get("offlineFlagSourcePath") { | ||
| options.source_configuration = Some(source.clone()); | ||
| if options.resolver_type != ResolverType::Rpc { | ||
| // For backwards compatibility: if offline path is set, switch to File resolver | ||
| // UNLESS resolver was explicitly set to "rpc" (in which case keep it as "rpc") | ||
| if !resolver_explicitly_set || options.resolver_type != ResolverType::Rpc { | ||
| options.resolver_type = ResolverType::File; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic here for determining the resolver type seems to contradict the stated goal of the pull request and the implementation in FlagdOptions::default().
The PR description states: "FLAGD_RESOLVER takes precedence. If set, it is respected and FLAGD_OFFLINE_FLAG_SOURCE_PATH does not override the resolver type to File."
The logic in FlagdOptions::default() correctly implements this by only overriding to File if FLAGD_RESOLVER is not set. However, the logic in this test is if !resolver_explicitly_set || options.resolver_type != ResolverType::Rpc, which is equivalent to if !(resolver_explicitly_set && options.resolver_type == ResolverType::Rpc). This means that setting offlineFlagSourcePath will force the resolver to File unless the resolver was explicitly set to Rpc. For example, if the resolver was explicitly set to InProcess, this logic would override it to File, which seems incorrect according to the PR's goal.
If the goal is to give any explicitly set resolver precedence, the condition should be simplified. If the current logic is required to pass the behavioral tests, it might be worth adding a more detailed comment explaining this specific backward-compatibility rule.
| if let Some(source) = world.option_values.get("offlineFlagSourcePath") { | |
| options.source_configuration = Some(source.clone()); | |
| if options.resolver_type != ResolverType::Rpc { | |
| // For backwards compatibility: if offline path is set, switch to File resolver | |
| // UNLESS resolver was explicitly set to "rpc" (in which case keep it as "rpc") | |
| if !resolver_explicitly_set || options.resolver_type != ResolverType::Rpc { | |
| options.resolver_type = ResolverType::File; | |
| } | |
| } | |
| if let Some(source) = world.option_values.get("offlineFlagSourcePath") { | |
| options.source_configuration = Some(source.clone()); | |
| // For backwards compatibility: if offline path is set, switch to File resolver | |
| // UNLESS resolver was explicitly set. | |
| if !resolver_explicitly_set { | |
| options.resolver_type = ResolverType::File; | |
| } | |
| } |
| match world.flag_type.as_str() { | ||
| "Boolean" => { | ||
| let default_bool = world.default_value.to_lowercase() == "true"; | ||
| let result = provider | ||
| .resolve_bool_value(&world.flag_key, &world.context) | ||
| .await; | ||
|
|
||
| match result { | ||
| Ok(details) => { | ||
| world.resolved_value = Some(details.value.to_string()); | ||
| world.resolved_reason = details.reason.map(reason_to_string); | ||
| world.resolved_variant = details.variant; | ||
| world.resolved_error_code = None; | ||
| } | ||
| Err(err) => { | ||
| world.resolved_value = Some(default_bool.to_string()); | ||
| world.resolved_reason = Some("ERROR".to_string()); | ||
| world.resolved_error_code = Some(format!("{:?}", err.code)); | ||
| } | ||
| } | ||
| } | ||
| "String" => { | ||
| let result = provider | ||
| .resolve_string_value(&world.flag_key, &world.context) | ||
| .await; | ||
|
|
||
| match result { | ||
| Ok(details) => { | ||
| world.resolved_value = Some(details.value.clone()); | ||
| world.resolved_reason = details.reason.map(reason_to_string); | ||
| world.resolved_variant = details.variant; | ||
| world.resolved_error_code = None; | ||
| } | ||
| Err(err) => { | ||
| world.resolved_value = Some(world.default_value.clone()); | ||
| world.resolved_reason = Some("ERROR".to_string()); | ||
| world.resolved_error_code = Some(format!("{:?}", err.code)); | ||
| } | ||
| } | ||
| } | ||
| "Integer" => { | ||
| let default_int = world.default_value.trim().parse::<i64>().unwrap_or(0); | ||
| let result = provider | ||
| .resolve_int_value(&world.flag_key, &world.context) | ||
| .await; | ||
|
|
||
| match result { | ||
| Ok(details) => { | ||
| world.resolved_value = Some(details.value.to_string()); | ||
| world.resolved_reason = details.reason.map(reason_to_string); | ||
| world.resolved_variant = details.variant; | ||
| world.resolved_error_code = None; | ||
| } | ||
| Err(err) => { | ||
| world.resolved_value = Some(default_int.to_string()); | ||
| world.resolved_reason = Some("ERROR".to_string()); | ||
| world.resolved_error_code = Some(format!("{:?}", err.code)); | ||
| } | ||
| } | ||
| } | ||
| "Float" => { | ||
| let default_float = world.default_value.trim().parse::<f64>().unwrap_or(0.0); | ||
| let result = provider | ||
| .resolve_float_value(&world.flag_key, &world.context) | ||
| .await; | ||
|
|
||
| match result { | ||
| Ok(details) => { | ||
| world.resolved_value = Some(details.value.to_string()); | ||
| world.resolved_reason = details.reason.map(reason_to_string); | ||
| world.resolved_variant = details.variant; | ||
| world.resolved_error_code = None; | ||
| } | ||
| Err(err) => { | ||
| world.resolved_value = Some(default_float.to_string()); | ||
| world.resolved_reason = Some("ERROR".to_string()); | ||
| world.resolved_error_code = Some(format!("{:?}", err.code)); | ||
| } | ||
| } | ||
| } | ||
| "Object" => { | ||
| let result = provider | ||
| .resolve_struct_value(&world.flag_key, &world.context) | ||
| .await; | ||
|
|
||
| match result { | ||
| Ok(details) => { | ||
| // Convert StructValue to JSON by converting fields | ||
| let json_obj: serde_json::Map<String, serde_json::Value> = details | ||
| .value | ||
| .fields | ||
| .iter() | ||
| .map(|(k, v)| (k.clone(), value_to_json(v))) | ||
| .collect(); | ||
| let json_str = | ||
| serde_json::to_string(&json_obj).unwrap_or_else(|_| "{}".to_string()); | ||
| world.resolved_value = Some(json_str); | ||
| world.resolved_reason = details.reason.map(reason_to_string); | ||
| world.resolved_variant = details.variant; | ||
| world.resolved_error_code = None; | ||
| } | ||
| Err(err) => { | ||
| world.resolved_value = Some(world.default_value.clone()); | ||
| world.resolved_reason = Some("ERROR".to_string()); | ||
| world.resolved_error_code = Some(format!("{:?}", err.code)); | ||
| } | ||
| } | ||
| } | ||
| _ => panic!("Unknown flag type: {}", world.flag_type), | ||
| } |
There was a problem hiding this comment.
There's a significant amount of duplicated logic in the match statement for handling the results of different resolve_*_value calls. The Ok and Err arms are nearly identical for each flag type. This could be refactored to improve maintainability.
Consider extracting the logic for handling Ok(details) and Err(err) into helper functions. This would make the evaluate_flag_with_details function shorter and easier to read. Note that the Object type might require special handling due to its conversion to a JSON string.
For example, you could have helpers like:
fn handle_ok<T>(world: &mut EvaluationWorld, details: open_feature::ResolutionDetails<T>, value_str: String) {
world.resolved_value = Some(value_str);
world.resolved_reason = details.reason.map(reason_to_string);
world.resolved_variant = details.variant;
world.resolved_error_code = None;
}
fn handle_err<T: ToString>(world: &mut EvaluationWorld, err: open_feature::EvaluationError, default_value: T) {
world.resolved_value = Some(default_value.to_string());
world.resolved_reason = Some("ERROR".to_string());
world.resolved_error_code = Some(format!("{:?}", err.code));
}This would help reduce the boilerplate in each match arm.
This PR
This change may be breaking to existing users
If both
FLAGD_OFFLINE_FLAG_SOURCE_PATHandFLAGD_RESOLVERwere set,FLAGD_OFFLINE_FLAG_SOURCE_PATHoverrode the resolver type to File, ignoringFLAGD_RESOLVER.What is done now:
FLAGD_RESOLVERtakes precedence. If set, it is respected andFLAGD_OFFLINE_FLAG_SOURCE_PATHdoes not override the resolver type to File.Add evaluation behavioral tests as well, did not require any change in implementation except error mapping with expectation
Related Issues
Improves #43
Notes
Follow-up Tasks
How to test